-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat: replace xdate and momentjs #2701
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
If you want to run a linter and/or code formatter on the entire repo when working on it, do this on a new branch and merge it separately. It's very hard and time consuming to actually review your feature/fix because there are formatting changes all over the place. |
Well, if the project had a proper setup, it would work with the files needed only or not need at all. I ran the command which goes for the entire project. It should have been set in a different manner, if that wasnt the intent. I had to run because I use biome as default in the IDE and that was affecting the code in different ways, that's why I said that lint and format was not properly setup in this project. How running a command in the project going to be a hinder? It's clearly not been used or properly setup. This PR might need an exception, as others would not have the problem anymore. I even made a suggestion in my last part of the description. If this and the other PR are merged, I can work on the third and improve what is possible on the setup. This PR took a good amount of work, it's not something that can just be easily re-done due to formatting or linting fixes. Ignore all that is not date related, as those will be related to format, lint or sorting. This PR introduces a very good amount of improvement to the quality of the code. And by the lib immutability, less probable to introduce future bugs related to date. I took a look, is not hard to review. Most refactoring and used from impl will be from You dont seem to be reviewer, why are you worried about it, anyhow? @x1mrdonut1x |
And how have you installed this PR changes? Seems to be in a different project. Plus unrelated to changes done. |
Edit:
All tests passing. Ready for review.
I suggest reading the whole description.
My other PR that need merging, it's a simple one. #2696 but unrelated to this one.
Hi,
I've refactored the code to have a single point encapsulating date functionality, that should make the code cleaner. It was a mess before, with a bunch of repetitions spread, making it harder to maintain or erven replace the implementation if that were ever needed.
https://day.js.org/
Momentjs is known to be a buggy library due to being mutable and with it's side effects. No point in supporting legacy code just because someone might want. But regardless, dayjs were designed as straight replacement.
They themselves recommend another solution https://github.com/arshaw/xdate/blob/master/README.md but thats 20kb, dayjs is only 2-3kb.
Here is a comparison between the previous and the new implementation.
XDate
Date
object but adds chainability and better parsing.Day.js
Summary Table
Edits:
As I was still refactoring code, there were just a bunch of code with possible side effects, making it harder to actually know what was going on when reading.
I run the lint fix in the whole project, there were a bunch of reformat.
Much of the code were not encapsulated, and it was heavily tied to the previous libraries, it made way more difficult to migrate the library.
It appears that some components have a month property in them, yet it was tied as a XDate type, yet in one of the components, it was used as date, yet specified as just a month. So, there could have been hidden bugs in this part. Or just bad variable naming.
There seem to be a bunch of eslint and types errors, unrelated to anything I did, making it not possible to compile the project. That is not properly setup.
Btw many places lack testing, some tests were breaking and there was no clue where the issue was.
When adding tests, one should add unit tests covering all parts of it before adding any other testing that covers a different layer. This should be enforced in the project.
At this point tests in the project are all mixed up with no distinction of what type is what.
There should be a separation of files and folders naming per test type. Then enforce the coverage in the CI.
You should check
LocaleConfig
new implemntation. At the moment all the user need is setting thedefaultLocale
, the translation/i18n are automatically done bydayjs
.https://day.js.org/docs/en/i18n/changing-locale
https://day.js.org/docs/en/customization/customization
https://day.js.org/docs/en/i18n/i18n - https://github.com/iamkun/dayjs/tree/dev/src/locale
There are many bad implementations in the code, they dont make any sense and are far from clear. But, at least in the date part, it should be way clearer of what is happening. Still the implementations using it, might not be. Though I have simplified some.
It's like there were no reviews when they were added to the code.
Any project should have full enforcements of setup. Yet lint and format seems all out of place.
I see that this project lacks
If these two PRs are merged, I can work on another to deal with this, except on the coderabbit, of course.